-
Notifications
You must be signed in to change notification settings - Fork 595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TOML] Add Syntax #4168
base: master
Are you sure you want to change the base?
[TOML] Add Syntax #4168
Conversation
Nice, appreciate the quick follow-up PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only wanted to take a "quick" glance while filling a time gap and did not review the tests.
datetime: | ||
# https://toml.io/en/v1.0.0#offset-date-time | ||
# https://datatracker.ietf.org/doc/html/rfc3339 | ||
- match: '{{date_time}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the date_time
variable intended to be re-used or extendable? I generally try to avoid defining capturing groups inside variable and then refering to them in captures
because it involves a lot of indirection, especially so with nested variable definitions, and is hard to debug. This also makes the variable hardly extendable because the number of capturing groups and their scope assigments are pre-defined by the base syntax.
I can see that the date
and time
segments are re-used as separate patterns below, so we could make an exception for those, but I also don't really think the reusability of these small variables is worth the confusion.
To illustrate, there is a large capturing group inside date_time
that contains the entire time part, including separators, but without mental gymnastics I cannot tell which scope this corresponds to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date_time
probably used once, only, but various capture groups are defined in underlying time
and date
as well, which is used multiple times.
The only other way would be to fully drop those 3 vars and build those patterns individually in corresponding - match:
statements, which would add up a bit of duplication, which was argued against in other situations in the past various times.
It is popping, so singular
Comments may start at any point without whitespace in front.
This commit... 1. applies `meta.section` to whole line, including leading and trailing whitespace so possibly applied background color covers full width of a view, which provides better UX. 2. applies `meta.brackets` to `[...]` itself to introduce a scope, which only covers that part.
Follow syntax specification naming.
Create corresponding table-value and inline-table-value contexts.
To follow official specs, this commit ensures to terminate inline tables at eol.
ba6ace1
to
409af7c
Compare
Make the keys green as values and the world complicated beyond necessary by introducing dedicated string contexts for section headers, key names and values so than each one can be scoped and treated individually.
Did others also voice their opinions? So far I've only seen myself advocating for I assume the color scheme of the screenshots is Mariana, i.e. ST default? Edit: Can we in the current implementation already strip comments at the beginnings of lines, e.g. for Python inline script metadata? |
Single oppinions have often been enough to act as veto. Just followed it, also with regards to #4171 Even though Git Config already suffers the "string" in section overlay. And with regards to mapping interpretation and attempt to create common scopes, this oppinion is justified and correct. I actually also started with them. |
Exactly. Looks good.
We really need a way to update Mariana ourselves or finally get SHQ to update it. The |
Resolves #4167
This PR adds new TOML default package.
Syntax takes some inspiration from https://packagecontrol.io/packages/TOML but has finally been re-written from scratch using specs from https://toml.io/en/
Reasons:
version 2 syntax definition
be less strict with regards to incomplete statements to avoid overeager illegal highlighting.
It is arguable if current state of illegal value highlighting is already too much.
design syntax primarily using named contexts with possible inheritance in mind
this implementation is about 30% faster (in its initial state)
Remarks:
Both, tables (aka. sections) and keys can consist of fully qualified identifiers specifying a path of hash table keys to final value.
is the same as
Both represent following JSON:
As discussed in #4167 it probably doesn't make sense to map section name and key name scopes to those of JSON representation as it would cause both being scoped
meta.mapping.key string
, which results in unexpected highlighting compared to GitConfig and INI syntax.As keys can consist of multiple elements, it is questionable whether
meta.mapping.key string
convention is the appropriate choise at all, because using different scoping schemas for sections and keys doesn't feel right and scoping individual elementsstring.unquoted
orstring.quoted
separated by period, doesn't add any value, too.Hence this PR starts with scoping whole...
[section.name]
=>meta.section entity.name.section
(known from INI and GitConfig)key.name
=>meta.mapping.key entity.name.key
(which was used earlier)Only quotation punctuation and
.
spearator is scoped on top of thoseentity
scopes.